-
Notifications
You must be signed in to change notification settings - Fork 102
Improve kernel supervisor wrapper to make environment more predictable and configurable #7797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
E2E Tests 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR upgrades the kernel supervisor to v0.1.40 and makes its environment and lifecycle more configurable and predictable.
- Introduces a
runInShell
flag to optionally start supervisor and kernels in login shells - Refactors the wrapper script to invoke
nohup
internally and avoid straynohup.out
files - Adds API endpoints to get/set server configuration (e.g. idle shutdown hours) and dynamic timeout updates
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
extensions/positron-supervisor/src/kcclient/model/serverStatus.ts | Add version and processId fields to the server status model |
extensions/positron-supervisor/src/kcclient/model/serverConfiguration.ts | Introduce the ServerConfiguration model |
extensions/positron-supervisor/src/kcclient/model/newSession.ts | Add runInShell flag to session creation |
extensions/positron-supervisor/src/kcclient/model/models.ts & .openapi-generator/FILES | Export the new configuration model |
extensions/positron-supervisor/src/kcclient/api/defaultApi.ts | Add getServerConfiguration and setServerConfiguration methods |
extensions/positron-supervisor/src/KallichoreSession.ts | Pass runInShell from settings into new sessions |
extensions/positron-supervisor/src/KallichoreAdapterApi.ts | Hook config changes for idle shutdown, refactor nohup logic, track real PID |
extensions/positron-supervisor/resources/supervisor-wrapper.sh | Support optional nohup , run in login shell, and cleaned redirection |
extensions/positron-supervisor/package.json & package.nls.json | Add kernelSupervisor.runInShell setting and bump kallichore version |
Comments suppressed due to low confidence (1)
extensions/positron-supervisor/src/kcclient/api/defaultApi.ts:452
- New methods
getServerConfiguration
andsetServerConfiguration
are added but there are no corresponding unit or integration tests. Adding tests will ensure these endpoints work as expected.
public async getServerConfiguration (options: {headers: {[name: string]: string}} = {headers: {}}) : Promise<{ response: http.IncomingMessage; body: ServerConfiguration; }> {
Co-authored-by: Copilot <[email protected]> Signed-off-by: Jonathan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating idle timeout
- I didn't test this using Remote SSH, but I confirmed that changing the timeout is reflected in the Kernel Supervisor output -- some sample output:
Updating server configuration with new shutdown timeout: 0
18:22:05 [INFO] Updating idle shutdown hours to None
Updating server configuration with new shutdown timeout: 4
18:22:07 [INFO] Updating idle shutdown hours to Some(4)
Updating server configuration with new shutdown timeout: -1
Failed to update idle timeout: HTTP 400. idle_shutdown_hours must be a non-negative integer
Updating server configuration with new shutdown timeout: 1
18:22:17 [INFO] Updating idle shutdown hours to Some(1)
- there seems to be an error when setting the timeout to
indefinitely
though -- I left a comment in the code where the error is caught
Windows PATH
- I did not test this 🤠
login shell change ✅
nohup no longer generated ✅
- on Mac, I set
"kernelSupervisor.shutdownTimeout": "4"
and restarted Positron - created a Python console session and ran the following code:
import time x = 10000 while x > 0: print(x) x-=1 time.sleep(2)
- quit Positron
- opened Activity Monitor, searched for
kcserver
and quit the process - opened Positron again
- no nohup.out in workspace (in 2025.06.0-107 I do get a nohup.out)
try { | ||
await this._api.setServerConfiguration({ | ||
idleShutdownHours: timeout | ||
}); | ||
} catch (err) { | ||
this._log.appendLine(`Failed to update idle timeout: ${summarizeError(err)}`); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're hitting the error case when the idle timeout is set to "indefinitely" -1
-- is it intentional to error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I forgot to update the supervisor side of this value! Thanks for catching that.
/** | ||
* Update the idle timeout on the server. This is used to set the idle | ||
* timeout on a server that has already started. | ||
*/ | ||
async updateIdleTimeout() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to stay because there are certain transitions that do require a restart. In particular we store sessions differently when you are in "immediate" mode (ephemeral storage) vs. one of the other modes (durable storage), and there is no logic to move sessions between these storage classes. So you could change from e.g. 4
to 8
hours w/o a restart, but going from immediate
to 4
does require a restart. I think this is probably too difficult to explain and we should just tell folks they always need to restart.
@sharon-wang this should be ready for you to check again -- I've updated the supervisor with a new version that correctly supports the client side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Output looks happy now 😄
21:34:54 [DEBUG] (3) kcserver::kernel_session: [session python-1c3a9172] Running kernel in login shell: /bin/zsh
Updating server configuration with new shutdown timeout: -1
21:35:12 [INFO] set_server_configuration - X-Span-ID: "f3a721d3-4ff2-4e19-8092-823c8ef79fb1"
21:35:12 [INFO] Updating idle shutdown hours to None
21:35:14 [INFO] client_heartbeat - X-Span-ID: "2165fdb8-eb25-4e50-a7cf-a95fd0017ee9"
Full test suite results - https://github.com/posit-dev/positron/actions/runs/15220852088 |
This change updates the kernel supervisor to 0.1.40, which, together with the Positron changes in this PR, addresses a number of related issues around the supervisor's lifecycle and environment.
The changes are as follows:
.bash_profile
. This behavior does have a minor impact on startup time, so I've made it optional.nohup
instead of the other way around. This inversion allows us to better control the behavior ofnohup
and prevent it from littering the project withnohup.out
files.PATH
to (non deterministically) be incorrect in R and Python sessions.Addresses #6757
Addresses #7215
Addresses #7274
Addresses #7537
Release Notes
New Features
.bash_profile
,.zprofile
) are now run when starting Python and R sessions (Supervisor: Run kernels in a login shell, or otherwise run startup scripts #6757)Bug Fixes
nohup.out
file littering projects when kernel supervisor is set to run after Positron exits (Filenohup.out
generated by Positron Supervisor #7215)QA Notes
Note that the supervisor and kernels are run in login shells, not interactive shells. This means that variables from
.bashrc
may not be visible; use e.g..bash_profile
instead. The system will attempt to use your default shell, which is zsh on recent macOS, so make sure you edit the profile script that matches your$SHELL
.